Bugfix: Add support for pull option in podman#23032
Bugfix: Add support for pull option in podman#23032tim-werner wants to merge 13 commits intopantsbuild:mainfrom
Conversation
|
ahh I have been afraid that this will happen. I need probably a new target type that except both but not sure if it will crash somewhere else than... |
|
This is one of those annoying problems. We have Podman support, which is "supposed" to be a mostly drop-in replacement for Docker, but in practice it isn't - and our implementation is overtly Docker-centric too. Can't wait to try on Apple Containers, on top of everything else. |
|
I can open a draft PR to show you my approach but I have been working on code to support building images directly with Buildkit sans Docker. To implement this, I added the concept of "engines" for the Docker backend, which lets you control which tool you use for builds and runs, but can definitely add push and pull as well. It uses separate This should let us easily add new OCI image platforms (like Apple containers) as well. |
|
Hi @ndellosa95 , |
|
Thanks for the contribution. We've just branched for 2.31.x, so merging this pull request now will come out in 2.32.x, please move the release notes updates to docs/notes/2.32.x.md if that's appropriate. |
|
@sureshjoshi finally had time to work again on it |
|
Alright, not working on my machine - but... I've also been doing some craziness. I'll try to dig through and see what comes up |
| # Boolean value | ||
| if is_podman: | ||
| # Convert boolean to Podman policy string | ||
| warnings.warn( |
There was a problem hiding this comment.
What was the reason to use warnings?
| return value_or_default | ||
|
|
||
|
|
||
| class StringOrBoolField(Field): |
There was a problem hiding this comment.
Hmm, I don’t particularly like this target. Is it strictly here to maintain backwards compatibility on the docker_image target?
| ) | ||
|
|
||
|
|
||
| def create_test_context(rule_runner: RuleRunner, pull_value=None): |
There was a problem hiding this comment.
The writing of this test feels a bit too clever, with all the conditionals. It looks less clean to maintain then maybe a single “file” template that can accept a value maybe.
I had to read it a couple times to actually understand it.
|
Thanks for the PR, but I feel like we’ve done a disservice to Podman by making it this weird, opt-in cousin to Docker - and that means we have to hack and patch our way to some more reasonable support. If it was all internals, that would be one thing as we could clean it up, but this is API level tweaks and they make me a bit nervous due to them being semi-locked in for a while. Just thinking out loud, but do we need a separate In other targets, we allow customization via |
| ) | ||
| docker_build_option = "--pull" | ||
|
|
||
| def options(self, value_formatter, global_build_hosts_options=None, **kwargs): |
There was a problem hiding this comment.
This function doesn’t feel like idiomatic Pants, but I don’t know exactly why.
4 participants
This pull request will fix issue: #21450
It is now possible to use booleans for Docker container for the
docker_imageoption--pullANDmissing,never,alwaysornewer. Using one of the strings and the Docker backend will result in an error. In the podman case, booleans are converted to podman policy strings - False =missingand True=always- to ensure backwards compatibility. I have added a deprecation warning in those cases.Disclaimer: AI was used for support, but all code was reviewed, adapted, and tested by a human.